fix(task-input): serve repo/branch selector from a persisted cache#3104
fix(task-input): serve repo/branch selector from a persisted cache#3104fercgomes wants to merge 2 commits into
Conversation
The cloud branch list had no cold-start cache: every app launch paid the full PostHog->GitHub round trip before the branch button showed anything but "Loading...", and opening the repo picker flashed "Loading repositories..." even though the full repo list was already in memory. Extend the cachedCloudRepositoryMap pattern to branches: persist the last-known first page of branches + default branch per repo (LRU-capped), serve it instantly while the live query revalidates in the background, and fall back to the already-loaded full repo list while the picker's search query loads its first page. Cache decisions are pure functions in @posthog/core with unit tests. GROW-73 Generated-By: PostHog Code Task-Id: 432c825c-dfe4-4b86-a3a0-c3217911b357
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
Reviews (1): Last reviewed commit: "fix(task-input): serve repo/branch selec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| describe("resolveRepositoryPickerList", () => { | ||
| const allRepositories = ["a/x", "a/y"]; | ||
| const pickerRepositories = ["a/x"]; | ||
|
|
||
| it("shows the full list while the picker is closed", () => { | ||
| const result = resolveRepositoryPickerList({ | ||
| pickerOpen: false, | ||
| pickerPending: true, | ||
| searchActive: false, | ||
| pickerRepositories, | ||
| allRepositories, | ||
| }); | ||
| expect(result).toEqual({ | ||
| repositories: allRepositories, | ||
| pickerLoading: false, | ||
| }); | ||
| }); | ||
|
|
||
| it("falls back to the full list while the first picker page loads", () => { | ||
| const result = resolveRepositoryPickerList({ | ||
| pickerOpen: true, | ||
| pickerPending: true, | ||
| searchActive: false, | ||
| pickerRepositories: [], | ||
| allRepositories, | ||
| }); | ||
| expect(result).toEqual({ | ||
| repositories: allRepositories, | ||
| pickerLoading: false, | ||
| }); | ||
| }); | ||
|
|
||
| it("shows the loading picker list while a search is pending", () => { | ||
| const result = resolveRepositoryPickerList({ | ||
| pickerOpen: true, | ||
| pickerPending: true, | ||
| searchActive: true, | ||
| pickerRepositories, | ||
| allRepositories, | ||
| }); | ||
| expect(result).toEqual({ | ||
| repositories: pickerRepositories, | ||
| pickerLoading: true, | ||
| }); | ||
| }); | ||
|
|
||
| it("keeps the loading state when there is no full list to fall back to", () => { | ||
| const result = resolveRepositoryPickerList({ | ||
| pickerOpen: true, | ||
| pickerPending: true, | ||
| searchActive: false, | ||
| pickerRepositories: [], | ||
| allRepositories: [], | ||
| }); | ||
| expect(result).toEqual({ repositories: [], pickerLoading: true }); | ||
| }); | ||
|
|
||
| it("shows the picker list once it settles", () => { | ||
| const result = resolveRepositoryPickerList({ | ||
| pickerOpen: true, | ||
| pickerPending: false, | ||
| searchActive: false, | ||
| pickerRepositories, | ||
| allRepositories, | ||
| }); | ||
| expect(result).toEqual({ | ||
| repositories: pickerRepositories, | ||
| pickerLoading: false, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
resolveRepositoryPickerList
All five cases here share an identical structure — a resolveRepositoryPickerList(inputs) call followed by expect(result).toEqual(expected) — but each is written as a separate non-parameterised test. The team's convention is to use it.each whenever the test body can be described by a table of inputs and expected outputs, which this group clearly can. Leaving them as individual it blocks adds test boilerplate without extra expressive power.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Review feedback: the five cases share a single call-and-compare shape, so express them as an it.each table per the repo's testing convention. Generated-By: PostHog Code Task-Id: 432c825c-dfe4-4b86-a3a0-c3217911b357
1 participant
Closes GROW-73
Problem
The task composer's repo/branch selector shows loading states for too long, especially on first open:
cachedCloudRepositoryMap, but the branch list paid the full PostHog → GitHub round trip on every app launch — the branch button sat on "Loading..." until it resolved, and the query couldn't even start until the repo list had loaded.Changes
Persisted branch cache (the main fix).
useUserGithubBranchesnow keeps the last-known branch list + default branch per repo in the settings store (cachedCloudBranchMap, next to the existing repo cache). On cold start or repo switch, the selector renders the cached list and auto-selects the cached default branch instantly while the live query revalidates behind the refresh spinner. Fresh results are written back after each successful fetch — unsearched first page only (50 branches, all the combobox renders anyway), LRU-capped at 20 repos, cleared when a repo cleanly reports no branches or integrations are removed. Most branches being worked on never change, so serving the stale list is almost always right.Repo picker open fallback. While the picker's search query loads its first page with no search typed, it falls back to the already-loaded full repository list instead of an empty loading state. Also adds keep-previous-data (
placeholderData) to the user repo search queries so typing doesn't blank the list (the org-level hook already did this).Cache decisions are pure functions in
@posthog/core(resolveEffectiveBranches,resolveBranchCacheUpdate,resolveRepositoryPickerList) with the hooks as thin glue, mirroring the existinguseUserRepositoryIntegrationpattern.Notes
staleTimealready allowed, just extended across restarts.pnpm typecheckacross all packages, Biome clean.🤖 Generated with Claude Code